Refactor Core Architecture & Eliminate Polling by SuperCoolPencil · Pull Request #522 · SurgeDM/Surge

SuperCoolPencil · 2026-07-01T08:48:54Z

Architecture Refactor: High Level Design

This document visually illustrates the recent structural changes to the Surge architecture, moving from a tightly coupled monolithic engine to a layered, domain-driven design.

The Old Architecture (Monolithic)

In the previous design, the internal/engine package acted as a catch-all monolith. The UI and Core tightly coupled to the engine's internal workings, and state was updated via direct callbacks and polling.

graph TD
    A[TUI / CLI] --> B[internal/core]
    B --> C[internal/processing]
    C --> D((internal/engine))
    
    subgraph Monolithic Engine
        D --> E[engine/concurrent]
        D --> F[engine/single]
        D --> G[engine/transport]
        D --> H[engine/state]
        D --> I[engine/types/progress.go]
    end
    
    E -.-> |Hooks / Polling| C
    F -.-> |Hooks / Polling| C

The New Architecture (Decoupled & Layered)

The new architecture introduces strict layering and unidirectional data flow. Components are isolated, and the monolithic engine has been decomposed into strategy, scheduler, orchestrator, progress, and store. The EventBus replaces direct callback hooks, and Service provides a clean API boundary.

graph TD
    UI[TUI / CLI] --> API[Service Layer<br>internal/service]
    
    API --> ORCH[Orchestrator Layer<br>internal/orchestrator]
    
    subgraph Core Domain
        ORCH --> SCHED[Scheduler Layer<br>internal/scheduler]
        SCHED --> STRAT[Strategy Layer<br>internal/strategy]
        
        STRAT --> C[concurrent]
        STRAT --> S[single]
        
        STRAT --> PROG[Progress Tracking<br>internal/progress]
        STRAT --> TRANS[Transport / Network<br>internal/transport]
    end
    
    STRAT -.-> |Publishes Typed Events| BUS[EventBus]
    PROG -.-> |Publishes Stats| BUS
    
    BUS -.-> |Subscribed Stream| API
    API -.-> |Real-time Updates| UI
    
    ORCH --> STORE[State / DB Layer<br>internal/store]

Download Lifecycle Flow

Here is how a download request flows through the new system:

sequenceDiagram
    participant U as User (UI/CLI)
    participant S as Service
    participant O as Orchestrator
    participant SCH as Scheduler
    participant W as Strategy (Worker)
    participant B as EventBus
    
    U->>S: Add(url)
    S->>O: Enqueue Download
    O->>B: Publish(State: Queued)
    O->>SCH: Submit Task
    
    SCH->>W: Execute(Strategy)
    W->>B: Publish(State: Downloading)
    
    loop During Download
        W->>W: Fetch Chunks
        W->>B: Publish(ProgressEvent)
    end
    
    W->>SCH: Complete/Error
    W->>B: Publish(State: Completed)
    SCH->>O: Task Done

Greptile Summary

This PR replaces the monolithic internal/engine package with a layered, domain-driven architecture: internal/strategy (download algorithms), internal/scheduler (task lifecycle), internal/orchestrator (coordination + EventBus), internal/progress (progress tracking), internal/store (state persistence), and internal/service (public API). Polling and direct callback hooks are eliminated in favour of a typed EventBus and a ProgressAggregator.

The new EventBus cleanly separates publishers from subscribers; Shutdown() drains the buffered input channel before closing listener channels, fixing the previous shutdown-ordering bug.
enqueueResolved now persists the master-list entry before pool.Add, eliminating the EventStarted/EventQueued status-overwrite race.
stateProgress in the TUI correctly narrows msg.State through *types.DownloadRecord → progress.CfgProgress, fixing the always-nil chunk-map rendering.

Confidence Score: 4/5

The core refactor is solid and the most critical shutdown, persistence, and type-assertion bugs from the previous round are all addressed; the sheer breadth of 154 changed files leaves some residual uncertainty about untested edge paths.

Every previously flagged defect that was reproducible from the diff has been corrected: the EventBus drains correctly on shutdown, the master-list entry is persisted before pool.Add eliminating the EventQueued/EventStarted race, getDetailPath takes an explicit dir argument closing the baseDir data-race, buildResumeConfig carries the full task/bitmap state, ResumeBatch no longer early-exits on a single corrupt gob, and stateProgress correctly narrows through *DownloadRecord. The remaining concern is the scale of the change — 154 files across six new packages — which makes it difficult to have full confidence that every interaction has been exercised by the existing tests.

internal/scheduler/scheduler.go (safeSendProgress dead else-branch), internal/orchestrator/progress.go (ProgressAggregator shutdown ordering relative to pool), and the broader interaction between the new strategy/concurrent and strategy/single packages and the updated scheduler worker loop.

Important Files Changed

Filename	Overview
internal/orchestrator/event_bus.go	New EventBus: drains InputCh via channel-close on Shutdown (no ctx.Done in the broadcast loop), correctly preventing the previous drain-race; pubWg guards prevent InputCh from being closed while Publish is in-flight.
internal/orchestrator/manager.go	Shutdown ordering is now correct (aggregator → pool.GracefulShutdown → eventBus.Shutdown); synchronous master-list persist before pool.Add eliminates the EventStarted/EventQueued ordering race.
internal/orchestrator/events.go	EventQueued handler now only writes if the entry is absent, guarding against status regression when EventStarted is already persisted; EventPaused fallback path correctly uses advanceRemainingTasks to keep chunk boundaries aligned.
internal/orchestrator/pause_resume.go	buildResumeConfig now correctly copies tasks, chunkBitmap, and actualChunkSize from savedState; ResumeBatch cold path loads the master list as a fallback for downloads without a detail-state gob, eliminating the nil-panic and the "not found" false-error for queued downloads.
internal/store/db.go	ensureDirs() now snapshots baseDir/configured under masterMu.RLock() before calling ensureDirsInternal; cleanupOnce closure captures the local dir variable (not the package-level baseDir), closing the previous data-race window.
internal/store/state.go	getDetailPath() now takes an explicit dir argument; all callers snapshot baseDir under masterMu before passing it, eliminating the previously flagged concurrent-read races. LoadStates uses errors.Join and no longer early-returns on first error.
internal/scheduler/scheduler.go	GracefulShutdown correctly drains taskChan with matching wg.Done() calls and polls until IsPausing() is false before closing progressDone; safeSendProgress blocking else-branch is effectively dead code for current call sites but remains a latent hazard if doneCh==nil.
internal/tui/update_events.go	stateProgress() now correctly narrows msg.State through *types.DownloadRecord → engineprogress.CfgProgress, fixing the always-nil chunk-map rendering from the previous wrong-type assertion.
internal/orchestrator/progress.go	New ProgressAggregator polls the scheduler's in-memory state on a 150 ms ticker and publishes EventBatchProgress; correctly skips paused and done downloads, and its Shutdown cancels the goroutine before pool.GracefulShutdown runs.
internal/service/local_service.go	ReloadSettings now correctly delegates to lifecycle.ApplySettings instead of returning nil unconditionally, fixing the previously flagged silent no-op.

Comments Outside Diff (12)

internal/orchestrator/pause_resume.go, line 211-232 (link)

ResumeBatch cold path fails for downloads without a detail state file

store.LoadStates(coldIDs) only reads from details/<id>.gob files written by SaveStateWithOptions (i.e., downloads that have been through at least one EventPaused cycle). Queued downloads that never started, and errored downloads that never saved a pause snapshot, have no detail file. For these IDs states[id] will be absent, and ResumeBatch returns "download not found or completed".

In contrast, the single Resume path calls store.GetDownload(id) first and succeeds even when LoadState fails, because buildResumeConfig accepts a nil savedState and falls back to the master-list entry. The batch path should do the same: supplement LoadStates with a batch master-list lookup and fall through to buildResumeConfig(id, outputPath, entry, nil, settings) for IDs not present in the detail map.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/orchestrator/pause_resume.go
Line: 211-232

Comment:
**`ResumeBatch` cold path fails for downloads without a detail state file**

`store.LoadStates(coldIDs)` only reads from `details/<id>.gob` files written by `SaveStateWithOptions` (i.e., downloads that have been through at least one `EventPaused` cycle). Queued downloads that never started, and errored downloads that never saved a pause snapshot, have no detail file. For these IDs `states[id]` will be absent, and `ResumeBatch` returns `"download not found or completed"`.

In contrast, the single `Resume` path calls `store.GetDownload(id)` first and succeeds even when `LoadState` fails, because `buildResumeConfig` accepts a `nil` `savedState` and falls back to the master-list `entry`. The batch path should do the same: supplement `LoadStates` with a batch master-list lookup and fall through to `buildResumeConfig(id, outputPath, entry, nil, settings)` for IDs not present in the detail map.

How can I resolve this? If you propose a fix, please make it concise.

internal/orchestrator/pause_resume.go, line 246-252 (link)

savedState is nil when a download lives only in the master list (queued but never started or saved a pause snapshot). At this point buildResumeConfig succeeds because it falls back to entry, but the immediately following savedState.Filename dereference panics. Any ResumeBatch call that includes a download that was queued and never executed (e.g., the app was killed before the scheduler ran it) will crash the process.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/orchestrator/pause_resume.go
Line: 246-252

Comment:
`savedState` is nil when a download lives only in the master list (queued but never started or saved a pause snapshot). At this point `buildResumeConfig` succeeds because it falls back to `entry`, but the immediately following `savedState.Filename` dereference panics. Any `ResumeBatch` call that includes a download that was queued and never executed (e.g., the app was killed before the scheduler ran it) will crash the process.



How can I resolve this? If you propose a fix, please make it concise.

internal/orchestrator/manager.go, line 331-342 (link)

Silently dropped EventQueued means download is lost on restart

Publish returns context.DeadlineExceeded after 1 second if InputCh is full; calling code discards the error with _ =. When this happens the download is already in the scheduler pool but StartEventWorker never sees EventQueued, so store.AddToMasterList is never called. The in-memory entry disappears on the next restart and the user has no indication the download was lost. The pool is also cleared by GracefulShutdown before state is written, so there's no recovery path.

InputCh can fill up when broadcastLoop is delayed by slow subscribers (each non-progress event waits up to 1 s per subscriber in broadcastMsg), making this reachable without a crash — a single lagging TUI or HTTP SSE client is sufficient.

Consider logging or surfacing a non-nil Publish error here, or using a direct store.AddToMasterList call as a synchronous fallback before the event is dispatched.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/orchestrator/manager.go
Line: 331-342

Comment:
**Silently dropped `EventQueued` means download is lost on restart**

`Publish` returns `context.DeadlineExceeded` after 1 second if `InputCh` is full; calling code discards the error with `_ =`. When this happens the download is already in the scheduler pool but `StartEventWorker` never sees `EventQueued`, so `store.AddToMasterList` is never called. The in-memory entry disappears on the next restart and the user has no indication the download was lost. The pool is also cleared by `GracefulShutdown` before state is written, so there's no recovery path.

`InputCh` can fill up when `broadcastLoop` is delayed by slow subscribers (each non-progress event waits up to 1 s per subscriber in `broadcastMsg`), making this reachable without a crash — a single lagging TUI or HTTP SSE client is sufficient.

Consider logging or surfacing a non-nil `Publish` error here, or using a direct `store.AddToMasterList` call as a synchronous fallback before the event is dispatched.

How can I resolve this? If you propose a fix, please make it concise.

internal/scheduler/manager.go, line 69-71 (link)

When all 100 candidate names are already occupied, uniqueFilePath silently returns the original path — which is known to conflict (the existence check at the top of the function confirmed it). Any caller expecting a non-conflicting path from this function would silently overwrite an existing file. The fallback should either return an error-sentinel empty string or append a timestamp/random suffix to guarantee a unique result.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/scheduler/manager.go
Line: 69-71

Comment:
When all 100 candidate names are already occupied, `uniqueFilePath` silently returns the original `path` — which is known to conflict (the existence check at the top of the function confirmed it). Any caller expecting a non-conflicting path from this function would silently overwrite an existing file. The fallback should either return an error-sentinel empty string or append a timestamp/random suffix to guarantee a unique result.



How can I resolve this? If you propose a fix, please make it concise.

cmd/root.go, line 114-124 (link)

Unguarded type assertion on ProgressState will panic on unexpected types

cfg.ProgressState.(*progress.DownloadProgress) is used without the comma-ok form in two places here. Because ProgressState is typed as interface{}, any value that is not *progress.DownloadProgress triggers a panic inside buildActiveDownloadChecker. The rest of the codebase consistently uses progress.CfgProgress(&cfg) for this narrowing, which handles nil and wrong-type gracefully — the new code should use the same helper.

Prompt To Fix With AI

This is a comment left during a code review.
Path: cmd/root.go
Line: 114-124

Comment:
**Unguarded type assertion on `ProgressState` will panic on unexpected types**

`cfg.ProgressState.(*progress.DownloadProgress)` is used without the comma-ok form in two places here. Because `ProgressState` is typed as `interface{}`, any value that is not `*progress.DownloadProgress` triggers a panic inside `buildActiveDownloadChecker`. The rest of the codebase consistently uses `progress.CfgProgress(&cfg)` for this narrowing, which handles nil and wrong-type gracefully — the new code should use the same helper.

How can I resolve this? If you propose a fix, please make it concise.

internal/store/db.go, line 31-35 (link)

Data race: configured and baseDir read without a lock in ensureDirs

Configure and CloseDB write configured and baseDir under masterMu, but ensureDirs reads both without holding any lock. When SaveStateWithOptions calls ensureDirs before acquiring masterMu, a concurrent Configure or CloseDB call races with these reads. The Go race detector will flag this. Holding masterMu.RLock() for the read-only check at the top of ensureDirs would close the race.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/store/db.go
Line: 31-35

Comment:
**Data race: `configured` and `baseDir` read without a lock in `ensureDirs`**

`Configure` and `CloseDB` write `configured` and `baseDir` under `masterMu`, but `ensureDirs` reads both without holding any lock. When `SaveStateWithOptions` calls `ensureDirs` before acquiring `masterMu`, a concurrent `Configure` or `CloseDB` call races with these reads. The Go race detector will flag this. Holding `masterMu.RLock()` for the read-only check at the top of `ensureDirs` would close the race.



How can I resolve this? If you propose a fix, please make it concise.

internal/store/db.go, line 43-48 (link)

Residual data race: cleanupOrphans(baseDir) reads package variable without a lock

The PR applied the previous review's suggestion by reading configured and baseDir under masterMu.RLock() and snapshotting them into isConfigured/dir — but the cleanupOnce.Do closure at line 45 reads the package variable baseDir directly instead of the captured dir. This is still a concurrent read without the lock.

Race scenario: Thread A releases masterMu.RUnlock() with dir="/path1", Thread B calls CloseDB (sets baseDir="") then Configure with /path2 (sets baseDir="/path2"), Thread A enters cleanupOnce.Do and calls cleanupOrphans("/path2") — cleaning temp files from an unrelated base directory. On Linux, if baseDir is "" when the closure fires, os.ReadDir("") traverses the current working directory and can delete any .tmp-* files found there.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/store/db.go
Line: 43-48

Comment:
**Residual data race: `cleanupOrphans(baseDir)` reads package variable without a lock**

The PR applied the previous review's suggestion by reading `configured` and `baseDir` under `masterMu.RLock()` and snapshotting them into `isConfigured`/`dir` — but the `cleanupOnce.Do` closure at line 45 reads the package variable `baseDir` directly instead of the captured `dir`. This is still a concurrent read without the lock.

Race scenario: Thread A releases `masterMu.RUnlock()` with `dir="/path1"`, Thread B calls `CloseDB` (sets `baseDir=""`) then `Configure` with `/path2` (sets `baseDir="/path2"`), Thread A enters `cleanupOnce.Do` and calls `cleanupOrphans("/path2")` — cleaning temp files from an unrelated base directory. On Linux, if `baseDir` is `""` when the closure fires, `os.ReadDir("")` traverses the current working directory and can delete any `.tmp-*` files found there.



How can I resolve this? If you propose a fix, please make it concise.

internal/orchestrator/manager.go, line 312-362 (link)

EventStarted can arrive in InputCh before EventQueued, causing status corruption

dispatchToScheduler at line 312 calls mgr.pool.Add(cfg), which may immediately schedule a pool worker. That worker calls safeSendProgress(cfg.ProgressCh, EventStarted) — writing directly to InputCh — before enqueueResolved reaches the store.AddToMasterList call at line 342 or the Publish(EventQueued) call at line 358. The broadcastLoop then serialises them for StartEventWorker in arrival order: EventStarted is processed first (sets status "downloading"), then EventQueued is processed second (unconditional AddToMasterList with Status: "queued" at events.go:331), overwriting the correct status. Any crash between the EventQueued write and the next lifecycle event (EventComplete/EventPaused/EventError) leaves the DB entry as "queued" for a download that had already started, preventing correct crash-recovery resume.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/orchestrator/manager.go
Line: 312-362

Comment:
**EventStarted can arrive in InputCh before EventQueued, causing status corruption**

`dispatchToScheduler` at line 312 calls `mgr.pool.Add(cfg)`, which may immediately schedule a pool worker. That worker calls `safeSendProgress(cfg.ProgressCh, EventStarted)` — writing directly to `InputCh` — before `enqueueResolved` reaches the `store.AddToMasterList` call at line 342 or the `Publish(EventQueued)` call at line 358. The `broadcastLoop` then serialises them for `StartEventWorker` in arrival order: `EventStarted` is processed first (sets status `"downloading"`), then `EventQueued` is processed second (unconditional `AddToMasterList` with `Status: "queued"` at `events.go:331`), overwriting the correct status. Any crash between the `EventQueued` write and the next lifecycle event (`EventComplete`/`EventPaused`/`EventError`) leaves the DB entry as `"queued"` for a download that had already started, preventing correct crash-recovery resume.

How can I resolve this? If you propose a fix, please make it concise.

internal/store/state.go, line 102-113 (link)

getDetailPath(state.ID) at line 106 reads the package-level baseDir variable without holding masterMu. Between the ensureDirs() call (which acquires and releases masterMu.RLock()) and the masterMu.Lock() at line 112, a concurrent CloseDB() or Configure() can modify baseDir. If CloseDB fires in that window, getDetailPath computes a path rooted at "", and atomicWrite creates or reads a file relative to the current working directory. The Go race detector will flag this. The fix is to hold the master lock for the entire write sequence.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/store/state.go
Line: 102-113

Comment:
`getDetailPath(state.ID)` at line 106 reads the package-level `baseDir` variable without holding `masterMu`. Between the `ensureDirs()` call (which acquires and releases `masterMu.RLock()`) and the `masterMu.Lock()` at line 112, a concurrent `CloseDB()` or `Configure()` can modify `baseDir`. If `CloseDB` fires in that window, `getDetailPath` computes a path rooted at `""`, and `atomicWrite` creates or reads a file relative to the current working directory. The Go race detector will flag this. The fix is to hold the master lock for the entire write sequence.



How can I resolve this? If you propose a fix, please make it concise.

internal/store/state.go, line 254-273 (link)

getDetailPath reads baseDir without holding masterMu in LoadStates

LoadStates is called entirely outside any lock. Inside the loop, getDetailPath(id) reads the package-level baseDir variable without any synchronization. If a concurrent CloseDB() fires between two iterations, baseDir becomes "" and getDetailPath returns "details/<id>.gob" — a relative path resolved against the current working directory. Any .gob file found there would be silently decoded and returned as valid resume state, corrupting the batch-resume result.

The same race exists in LoadState at line 243 (loadGob(getDetailPath(foundID), &ds)), which executes after LoadMasterList() has released its RLock. Both functions should snapshot baseDir under the read-lock before constructing the path, or delegate path construction to a caller-provided dir argument the way ensureDirsInternal does.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/store/state.go
Line: 254-273

Comment:
**`getDetailPath` reads `baseDir` without holding `masterMu` in `LoadStates`**

`LoadStates` is called entirely outside any lock. Inside the loop, `getDetailPath(id)` reads the package-level `baseDir` variable without any synchronization. If a concurrent `CloseDB()` fires between two iterations, `baseDir` becomes `""` and `getDetailPath` returns `"details/<id>.gob"` — a relative path resolved against the current working directory. Any `.gob` file found there would be silently decoded and returned as valid resume state, corrupting the batch-resume result.

The same race exists in `LoadState` at line 243 (`loadGob(getDetailPath(foundID), &ds)`), which executes after `LoadMasterList()` has released its `RLock`. Both functions should snapshot `baseDir` under the read-lock before constructing the path, or delegate path construction to a caller-provided `dir` argument the way `ensureDirsInternal` does.

How can I resolve this? If you propose a fix, please make it concise.

internal/store/state.go, line 106 (link)

getDetailPath reads baseDir without masterMu in three call sites

ensureDirs() at line 102 acquires and releases masterMu.RLock() and then returns. At line 106, getDetailPath(state.ID) reads the package-level baseDir variable again — this time without any lock. A concurrent CloseDB() or Configure() call between the two reads can change baseDir, causing atomicWrite to create or read a .gob file relative to an unintended directory (or the current working directory when baseDir is "").

The same race exists in LoadState (calls getDetailPath after LoadMasterList() releases its RLock) and in LoadStates (calls getDetailPath in a loop, entirely lock-free). The Go race detector will flag all three.

Fix: snapshot baseDir under masterMu.RLock() (or hold it) before calling getDetailPath, as already done in ensureDirs and DeleteState.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/store/state.go
Line: 106

Comment:
**`getDetailPath` reads `baseDir` without `masterMu` in three call sites**

`ensureDirs()` at line 102 acquires and releases `masterMu.RLock()` and then returns. At line 106, `getDetailPath(state.ID)` reads the package-level `baseDir` variable again — this time without any lock. A concurrent `CloseDB()` or `Configure()` call between the two reads can change `baseDir`, causing `atomicWrite` to create or read a `.gob` file relative to an unintended directory (or the current working directory when `baseDir` is `""`).

The same race exists in `LoadState` (calls `getDetailPath` after `LoadMasterList()` releases its `RLock`) and in `LoadStates` (calls `getDetailPath` in a loop, entirely lock-free). The Go race detector will flag all three.

Fix: snapshot `baseDir` under `masterMu.RLock()` (or hold it) before calling `getDetailPath`, as already done in `ensureDirs` and `DeleteState`.

How can I resolve this? If you propose a fix, please make it concise.

internal/scheduler/scheduler.go, line 638-662 (link)

safeSendProgress blocks indefinitely during shutdown if InputCh is full

safeSendProgress is a raw blocking send with no timeout or context. When a single-threaded download is paused during GracefulShutdown, the worker calls safeSendProgress(localCfg.ProgressCh, ...) before calling p.wg.Done(). If eventBus.InputCh (capacity 100) is completely full at that moment — possible when many downloads are pausing simultaneously while the broadcast loop is delayed by slow subscribers (each non-progress event waits up to 1 second per subscriber in broadcastMsg) — the worker goroutine blocks here indefinitely. pool.wg.Wait() never returns, and GracefulShutdown hangs forever.

The concurrent downloader avoids this by sending EventPaused before RunDownload returns nil, so the blocking path only affects single-threaded downloads. Consider replacing the raw send with a select that also reads from the existing progressDone channel (already used for shutdown signalling in the scheduler), so the worker can bail out if the bus is unresponsive.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/scheduler/scheduler.go
Line: 638-662

Comment:
**`safeSendProgress` blocks indefinitely during shutdown if `InputCh` is full**

`safeSendProgress` is a raw blocking send with no timeout or context. When a single-threaded download is paused during `GracefulShutdown`, the worker calls `safeSendProgress(localCfg.ProgressCh, ...)` before calling `p.wg.Done()`. If `eventBus.InputCh` (capacity 100) is completely full at that moment — possible when many downloads are pausing simultaneously while the broadcast loop is delayed by slow subscribers (each non-progress event waits up to 1 second per subscriber in `broadcastMsg`) — the worker goroutine blocks here indefinitely. `pool.wg.Wait()` never returns, and `GracefulShutdown` hangs forever.

The concurrent downloader avoids this by sending `EventPaused` before `RunDownload` returns nil, so the blocking path only affects single-threaded downloads. Consider replacing the raw send with a `select` that also reads from the existing `progressDone` channel (already used for shutdown signalling in the scheduler), so the worker can bail out if the bus is unresponsive.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (18): Last reviewed commit: "refactor: ensure safe event bus shutdown..." | Re-trigger Greptile}

…rker notification

…oduce event-driven orchestrator components

…d ProgressAggregator components

…ing logic

…tatic-analysis linting for Unicode literals

…te fields for improved type consistency

…and implementing a bitmask-based tracker

github-actions · 2026-07-01T08:49:34Z

Binary Size Analysis

⚠️ Size Increased

Version	Human Readable	Raw Bytes
Main	17.00 MB	`17826084`
PR	17.02 MB	`17842468`
Difference	16.00 KB	`16384`

…ng to use atomic total size

…ation

…rmatting

…lidation using master list data

…d requests

…d resume event publish

…m Add and AddWithID service methods

…tenv while cleaning up orchestrator test constants

…blish method

…d update event handling logic

… filename collision exhaustion

… of direct database access

…files

…wnloadProgress

…r wait threshold for 429 concurrency tests

…migration

…ertions, and implement synchronous download persistence

…aster list versioning

…ed dir variable

…in store backend

…assertion robustness in test suite

…event race conditions during initialization

…ic path remapping tests

…d persistence by checking for existence first

- cmd/rm.go: fix errcheck lint violations by wrapping resp.Body.Close() in anonymous funcs so the error is explicitly discarded - internal/utils/debug.go: add CloseDebug() to close the open log file handle and reset sync.Once; protect Debug() writes with debugMu so CloseDebug() and Debug() are race-safe - cmd/test_env_test.go: call utils.CloseDebug() in setupXDGEnvIsolation cleanup so Windows can remove the temp dir (the open debug log file was causing 'file in use' errors during t.TempDir() cleanup) - cmd/cli_test.go: change net.Listen('tcp', ...) to 'tcp4' and replace t.Fatalf with t.Skipf on bind failure; mirrors the existing testutil.NewHTTPServerT behavior for runners where the dual-stack socket provider is unavailable - cmd/get_test.go: replace httptest.NewServer (which tries IPv6 first and panics) with testutil.NewHTTPServerT in startAuthedTestServer; add testutil import, remove unused httptest import

…ess test TestHandleDownload_HeadlessMode_AutoApprovesNonDuplicate used httptest.NewServer for the probe server, which tries to bind an IPv6 listener first and panics on Windows CI runners where the dual-stack socket service provider is unavailable. Replace with testutil.NewHTTPServerT (tcp4, skips on bind failure) to match the pattern used by the other tests fixed in the previous commit.

…HTTPServerT Four more bare httptest.NewServer calls were using Go's newLocalListener() which tries tcp6 [::1]:0 first and panics on Windows CI runners lacking the dual-stack socket provider: - cmd/http_api_test.go:259 TestEventsEndpoint_RequiresAuthAndStreamsSSE - cmd/http_api_test.go:619 TestExecuteAPIAction_SendsIDAsQueryParam - cmd/http_handler_test.go:300 (probe server for download handler test) - cmd/http_handler_test.go:435 (probe server for download handler test) Replace all with testutil.NewHTTPServerT (binds tcp4, skips gracefully on failure). Add testutil import to both files.

In a recent commit, the \Start\ method of \program\ was updated to forcibly set \os.Args\ to \server start\ to ensure the daemon mode starts correctly from the service manager. However, this means \TestProgramLifecycle\ and \TestProgramContextCancellation\ now actually run the background server logic during tests instead of executing a harmless \--help\ run. This surfaces the same dual-stack \ cp\ provider error on \windows-latest\ as the HTTP API tests when the server attempts to bind to a port, leading to \could not find available port\. Add a pre-check to gracefully skip both tests if the \ cp\ socket provider fails to initialize, matching the skip pattern established in \cli_test.go\ and \get_test.go\.

…n shutdown

…annel synchronization in scheduler

…ng and channel draining

SuperCoolPencil added 19 commits June 29, 2026 15:17

feat: replace ticker-based balancer polling with event-driven idle wo…

5a67386

…rker notification

refactor: Create internal/types and merge engine/types and engine/events

e81c9fc

refactor: Create internal/store from engine/state

2dd8c04

refactor: Create internal/progress from types

1dbb6fc

refactor: Create internal/transport from engine root files

6e57337

refactor: Create internal/strategy/concurrent and single

cc3e58f

refactor: Create internal/scheduler and delete download

5fc52e3

refactor: Create internal/service from core

d59dc86

refactor: remove extensive legacy unit and integration tests and intr…

475f849

…oduce event-driven orchestrator components

test: add comprehensive unit tests for EventBus, LifecycleManager, an…

8c7bc0f

…d ProgressAggregator components

test: rewrite unit tests for local and remote service layer (Phase 2)

a6a0ea6

test: implement comprehensive unit test suite and add rate limit pars…

e35dc47

…ing logic

add comprehensive test suite across internal packages and implement s…

edbee2e

…tatic-analysis linting for Unicode literals

refactor: remove obsolete test suite and cleanup legacy test files

8d22e06

chore: remove temporary migration script rename_state_to_store.sh

132b6b4

Phase 2: Complete migration to strictly typed DownloadEvent

2dbe0c4

Phase 2: Finalize test mocks for typed events

39e9dfe

refactor: rename DownloadConfig to DownloadRecord and consolidate sta…

d963b9c

…te fields for improved type consistency

refactor: decouple progress state by introducing internal structures …

1c19a11

…and implementing a bitmask-based tracker

greptile-apps Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread internal/orchestrator/manager.go

Comment thread internal/orchestrator/manager.go Outdated

Comment thread internal/service/local_service.go

Comment thread internal/types/events.go Outdated

SuperCoolPencil added 3 commits July 1, 2026 14:42

refactor: improve event bus thread safety and convert progress tracki…

d5ffbe4

…ng to use atomic total size

test: update downloader test to use atomic load for TotalSize verific…

5fc4d36

…ation

refactor: move CfgProgress helper to progress package and clean up fo…

0a99538

…rmatting

greptile-apps Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread internal/orchestrator/event_bus.go Outdated

SuperCoolPencil added 5 commits July 1, 2026 15:45

refactor: implement event bus shutdown drainage and enhance resume va…

c732ff7

…lidation using master list data

feat: allow manual override of TotalSize and SupportsRange in downloa…

f38f5a4

…d requests

fix: use configuration filename instead of saved state during downloa…

7b213bf

…d resume event publish

refactor: remove redundant totalSize and supportsRange parameters fro…

79d607a

…m Add and AddWithID service methods

fix: return empty master list instead of error on version mismatch

f43b051

SuperCoolPencil added 3 commits July 1, 2026 16:44

refactor: improve test reliability by migrating to t.Cleanup and t.Se…

16fd71c

…tenv while cleaning up orchestrator test constants

refactor: remove redundant block scope in countingLifecycleService Pu…

483a673

…blish method

refactor: use errors.Is for error comparisons, cleanup whitespace, an…

25f1199

…d update event handling logic

greptile-apps Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread internal/orchestrator/pause_resume.go

feat: include task state in DownloadRecord and return empty string on…

25c244e

… filename collision exhaustion

greptile-apps Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread internal/orchestrator/pause_resume.go Outdated

SuperCoolPencil added 2 commits July 2, 2026 10:03

refactor: migrate download cleanup commands to use remote API instead…

6bdd5e2

… of direct database access

fix: allow ResumeBatch to recover by ignoring corrupt download state …

acf9225

…files

greptile-apps Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread internal/tui/update_events.go

SuperCoolPencil added 21 commits July 2, 2026 16:40

refactor: update progress mapping to use DownloadRecord instead of Do…

a9b00c2

…wnloadProgress

test: include SystemRoot environment variable and increase Retry-Afte…

306a4cc

…r wait threshold for 429 concurrency tests

chore: update package-lock.json dependencies

634a846

chore: add pretest script to run wxt prepare before vitest

b33d3f2

test: update integration test to match camelCase to snake_case field …

cf91657

…migration

Merge branch 'main' into eliminate-polling

eb495ce

refactor: fix concurrency safety in store, simplify progress type ass…

9ae9ffa

…ertions, and implement synchronous download persistence

feat: add startup warning for config load errors and enforce strict m…

cb326de

…aster list versioning

fix: update cleanupOrphans base directory reference to use the provid…

e869fbd

…ed dir variable

fix: handle EventRemoved state cleanup and optimize directory access …

558e418

…in store backend

test: skip incompatible permission tests on Windows and improve path …

38ab7b5

…assertion robustness in test suite

refactor: decouple download record creation from pool insertion to pr…

e5b66ed

…event race conditions during initialization

fix: allow absolute paths on Windows daemons and skip platform-specif…

35be707

…ic path remapping tests

fix: prevent overwriting advanced download statuses during EventQueue…

327ddbc

…d persistence by checking for existence first

fix(scheduler): prevent safeSendProgress from blocking indefinitely o…

83ac05e

…n shutdown

refactor: thread-safe path resolution for detail files and improve ch…

473c851

…annel synchronization in scheduler

refactor: ensure safe event bus shutdown by adding publication tracki…

c86f222

…ng and channel draining

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Refactor Core Architecture & Eliminate Polling#522

Refactor Core Architecture & Eliminate Polling#522
SuperCoolPencil wants to merge 56 commits into
mainfrom
eliminate-polling

SuperCoolPencil commented Jul 1, 2026 •

edited by greptile-apps Bot

Loading

Uh oh!

github-actions Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

SuperCoolPencil commented Jul 1, 2026 • edited by greptile-apps Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Architecture Refactor: High Level Design

The Old Architecture (Monolithic)

The New Architecture (Decoupled & Layered)

Download Lifecycle Flow

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Comments Outside Diff (12)

Uh oh!

github-actions Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Analysis

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

SuperCoolPencil commented Jul 1, 2026 •

edited by greptile-apps Bot

Loading

github-actions Bot commented Jul 1, 2026 •

edited

Loading